-
Notifications
You must be signed in to change notification settings - Fork 113
feat(redistribution): update slashers #492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
src/slashers/InstantSlasher.sol
Outdated
operators[0] = _slashingParams.operator; | ||
slashingRegistryCoordinator.updateOperators(operators); | ||
_fulfillSlashingRequest(_slashingParams); | ||
_updateOperatorStakeWeights(_slashingParams.operator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a redistributable slasher which fulfills slash then redistributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not opposed, added _fulfillSlashingRequestAndBurnOrRedistribute
already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to yash's suggestion here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted to add the internal _fulfillSlashingRequestAndBurnOrRedistribute
method to the base. I don't think we should have an independent "Redistributable" Slasher that only calls this internal as its possible for it to fail and create a DoS scenario. If anything I think all slashers should expose both methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But wouldn't AVSs want this is a key feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel like its useful to have this in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -44,7 +44,7 @@ Creates and queues a new slashing request that will be executable after the veto | |||
#### `cancelSlashingRequest` | |||
```solidity | |||
function cancelSlashingRequest( | |||
uint256 requestId | |||
uint256 slashId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rename the request to the slash here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of redistribution is each slash now has a slashId
that's returned from slashOperator
. So there's no longer a need to track requestIds
in these slashers as its now duplicate storage.
4ecc5f8
to
716ebb3
Compare
Storage CI failing due to deprecating a variable. |
Motivation:
Explain here the context, and why you're making that change. What is the problem you're trying to solve.
Modifications:
Describe the modifications you've done.
Result:
After your change, what will change.